Skip to content

Conversation

@jmgasper
Copy link
Collaborator

Rush on this one due to support request

@jmgasper jmgasper requested a review from kkartunov as a code owner January 28, 2026 22:46
gap: 6px;
}

.dialogLoadingSpinnerContainer {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ design]
Using position: absolute for .dialogLoadingSpinnerContainer may cause layout issues if the parent container's dimensions or position change. Consider using a more flexible layout strategy if the spinner's position is not fixed relative to the viewport or other elements.

@jmgasper jmgasper merged commit caf0d88 into dev Jan 28, 2026
6 checks passed
props.setOpen(false)
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [isLoading])

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ correctness]
The handleClose function depends on isLoading, but props.setOpen is also used inside it. Consider adding props.setOpen to the dependency array to ensure the function is updated correctly if props.setOpen changes.

setShowConfirm(false)
setIsLoading(true)
const data = getValues()
const nextHandle = (data.newHandle ?? '').trim()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[💡 maintainability]
The nextHandle is trimmed before being passed to changeUserHandle, but the form validation schema should ensure that the handle is already in the correct format. Consider validating and trimming the handle in the schema to ensure consistency and avoid redundant operations.

.then(result => {
setIsLoading(false)
toast.success('Handle updated successfully')
props.userInfo.handle = result?.handle ?? nextHandle

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ design]
Directly mutating props.userInfo.handle could lead to unexpected behavior if userInfo is used elsewhere. Consider using a state update or a callback to update the handle in a more controlled manner.

@@ -309,6 +313,8 @@ export const UsersTable: FC<Props> = props => {
function onSelectOption(item: string): void {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ maintainability]
Consider using a switch statement instead of multiple else if conditions for better readability and maintainability. This will make it easier to add or modify options in the future.

newHandle: string,
): Promise<MemberInfo> => {
const payload = {
newHandle: newHandle.trim(),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ correctness]
Consider validating newHandle before trimming and sending it in the payload. This could prevent sending invalid data to the API and improve error handling.

}

return xhrPatchAsync<typeof payload, MemberInfo>(
`${EnvironmentConfig.API.V6}/members/${encodeURIComponent(handle)}/change_handle`,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[❗❗ security]
Ensure that handle is validated before being used in the URL. This prevents potential issues with malformed URLs or unintended API calls.

*/
export const formEditUserHandleSchema: Yup.ObjectSchema<FormEditUserHandle>
= Yup.object({
newHandle: Yup.string()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ correctness]
Consider adding a .min(3, 'Handle must be at least 3 characters long.') constraint to ensure the handle is not too short, which could help prevent potential issues with handle uniqueness or readability.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants